Add field selector to list experiments API #44

deadlycoconuts · 2022-10-17T08:12:47Z

What this PR does / why we need it:
This PR introduces a field selector to the ListExperiments API of the management service; previously, field selectors only existed for the ListTreatments and ListSegments APIs.

The introduction of this field selector would allow more selective control over the fields of the experiment objects to return. Doing so would coincidentally allow the ListExperimentsTable of the UI to display larger numbers of experiments (>10) in each page, which it was previously unable to do because querying the API for an entire list of experiments would have resulted in too much data being transferred (due to the unnecessary fields/columns being returned).

As such, this PR also makes a tiny change to the UI to activate the option to allow users to select the number of experiments they wish to display in each page of the table. This change also affects the ListTreatmentsTable and ListSegmentsTable.

Modifications

api/experiments.yaml - Addition of the fields query field to the ListExperiments API
api/schema.yaml - Addition of the ExperimentField schema
management-service/models/experiment.go - Modification of the ToApiSchema method to return only selected fields
management-service/pagination/pagination.go - Modification of the existing MaxPageSize from 10 to 50
management-service/services/experiment_service.go - Modification of the ListExperiments method to handle the fields, and minor refactoring of the start/end time filter handling into a separate helper method
ui/src/components/table/BasicTable.js - Toggling of the prop to show page options in experiment/treatment/segment tables
ui/src/config.js - Addition of new configuration to specify the number of pages/fields to query when using the ListExperiments API
ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js - Minor refactoring of the existing process to retrieve the status (friendly) of an experiment

deadlycoconuts · 2022-10-19T02:12:36Z

management-service/services/experiment_service.go

@@ -578,6 +588,35 @@ func (svc *experimentService) filterExperimentStatusFriendly(query *gorm.DB, sta
 	return query.Where(orPredicates)
 }

+func (svc *experimentService) filterStartEndTimeValues(query *gorm.DB, params ListExperimentsParams) (*gorm.DB, error) {


Created a helper method out of this because the linter was throwing out an excessive cyclomatic complexity warning for the ListExperiments method.

Thanks for this!

krithika369 · 2022-10-19T07:10:11Z

api/schema.yaml

+        - start_time
+        - end_time
+        - updated_at
+        - treatments
    Experiment:


With the change in this PR, all fields from Experiment should be considered to be not required, like we do for Treatment.

krithika369 · 2022-10-19T07:27:21Z

management-service/services/experiment_service.go

@@ -172,7 +166,7 @@ func (svc *experimentService) ListExperiments(
 	// Pagination
 	var pagingResponse *pagination.Paging
 	var count int64
-	err := pagination.ValidatePaginationParams(params.Page, params.PageSize)
+	err = pagination.ValidatePaginationParams(params.Page, params.PageSize)


Can we make the behavior same as the list treatments api so that pagination is only applied if:

One of the panigation props is set

Fields is nil

This will allow bypassing pagination entirely if the Fields selector is used and for things like querying scheduled / active experiments in the standard ensembler UI for Turing, we can end up using a single API call.

Ref:
https://github.com/caraml-dev/xp/blob/main/management-service/services/treatment_service.go#L111

Oh my good catch! Thanks for the reminder; can't remember why I left this out 😵‍💫

krithika369 · 2022-10-19T07:30:18Z

management-service/services/experiment_service.go

@@ -578,6 +588,35 @@ func (svc *experimentService) filterExperimentStatusFriendly(query *gorm.DB, sta
 	return query.Where(orPredicates)
 }

+func (svc *experimentService) filterStartEndTimeValues(query *gorm.DB, params ListExperimentsParams) (*gorm.DB, error) {


Thanks for this!

deadlycoconuts · 2022-10-25T09:24:51Z

treatment-service/integration-test/fetch_treatment_it_test.go

@@ -811,5 +812,5 @@ func (suite *TreatmentServiceTestSuite) TestLocalStorage() {

 	resp, err := suite.managementServiceClient.GetExperimentWithResponse(suite.ctx, 1, 1)
 	suite.Require().NoError(err)
-	suite.Require().Equal(resp.JSON200.Data.Name, storage.Experiments[1][0].Experiment.Name)
+	suite.Require().Equal(storage.Experiments[1][0].Experiment.Name, *resp.JSON200.Data.Name)


Reordered the 2 arguments because the expected argument is supposed to come before the actual one.

deadlycoconuts · 2022-10-25T09:25:14Z

treatment-service/models/storage_test.go

@@ -110,7 +117,7 @@ func (suite *LocalStorageLookupSuite) SetupTest() {
 		Return(&http.Response{
 			StatusCode: 200,
 			Header:     map[string][]string{"Content-Type": {"json"}},
-			Body:       ioutil.NopCloser(bytes.NewBufferString(`{"data" : []}`)),
+			Body:       io.NopCloser(bytes.NewBufferString(`{"data" : []}`)),


Replaced the deprecated method with its replacement.

… not specified

deadlycoconuts · 2022-10-26T06:31:24Z

management-service/services/experiment_service.go

+		for k := range fieldNamesSet {
+			fieldNames = append(fieldNames, k)
+		}
+		query = query.Select(fieldNames)


This fieldFieldValues function essentially specifies SELECT values in the DB query such that only columns corresponding to the field values requested get retrieved.

deadlycoconuts · 2022-10-26T06:34:19Z

management-service/services/experiment_service.go

+			if field == models.ExperimentFieldStatusFriendly {
+				// Add ExperimentFieldStartTime and ExperimentFieldEndTime to the query as they are required for
+				// determining the statusFriendly field; these two fields may or may not be returned depending on
+				// what is actually specified in params.Fields
+				fieldNamesSet[string(models.ExperimentFieldStartTime)] = struct{}{}
+				fieldNamesSet[string(models.ExperimentFieldEndTime)] = struct{}{}
+
+				fieldName = models.ExperimentFieldStatus


This block is added since status_friendly does not exist in the DB as a column. In order to determine what this field value should be, we'll need to retrieve other columns, such as status, start_time and end_time.

The fieldNamesSet used to store column names ensure that only unique values are used in the query i.e. so we don't do something like SELECT start_time, status, start_time, ... FROM experiments (in SQL)

deadlycoconuts · 2022-10-26T06:45:37Z

management-service/services/experiment_service_test.go

 	// All experiments under a settings
-	expResponsesList, pagingResponse, err := svc.ListExperiments(1, services.ListExperimentsParams{})
+	actualResponsesList, pagingResponse, err := svc.ListExperiments(1, services.ListExperimentsParams{})


Renamed this as it's clearly the actual response, and not the expected response.

…ontroller behaviour

krithika369

Left some small comments. LGTM. Thanks for this PR! Please feel free to merge once you've reviewed / addressed the comments.

krithika369 · 2022-10-31T09:03:22Z

management-service/services/experiment_service.go

+		}
+
+		// Stores a map of unique field names which are unique column names to be selected in the db query
+		fieldNamesSet := make(map[string]struct{})


Why not use "github.com/golang-collections/collections/set" for this, which we commonly use elsewhere in the codebase?

Ahh, I tried doing that but I realised we can't extract the values contained within the Set as a slice of strings because the underlying field (hash) containing all the values added to the Set is actually a private field:

type ( Set struct { hash map[interface{}]nothing } nothing struct{} )

Hmm there should be a function to get it out.

https://github.com/golang-collections/collections/blob/604e922904d3/set/set.go#L36

They seem to all return a *Set at most 😕 https://pkg.go.dev/github.com/golang-collections/collections/set#pkg-index

An alternative is to run Do with a function that stores all the string values in the set into some slice.

krithika369 · 2022-10-31T09:21:28Z

ui/src/config.js

@@ -36,6 +36,11 @@ export const appConfig = {
    : [{ href: "https://github.com/caraml-dev/xp", label: "XP User Guide" }],
  pagination: {
    defaultPageSize: 10,
+    experimentContextPageSize: 50,
+  },
+  listExperimentFields: {


I'm not sure this should be an app-level config. Can it just be embedded in the components that use them? Reasons:

If this were an app-config, I think it would make sense to let it drive the behavior of the components entirely (like modifying the experimentTableFields here could also change the columns displayed in the experiments table, which is not the case now and I think that would be over-engineering at this point).

These configs are not re-used in multiple places which could also be a hint that this can be localised.

If either of the above reasons changed, we could consider extracting them out again but perhaps we will need to think more about what influence such configs will have (bullet # 1 above).

That's true, I was thinking of #1 as a distant possibility but yeah now that you mention it, it's probably over-engineering; I've removed both configs and directly embedded the lists in the components they are used.

krithika369 · 2022-10-31T09:48:14Z

ui/src/providers/experiment/context.js

  return (
    <ExperimentContext.Provider
      value={{
-        allExperiments,
-        isAllExperimentsLoaded,
+        experiments,


Should we rename this to scheduledAndRunningExperiments or something to be more specific? Although the ExperimentContextProvider is only used in the Turing microfrontend components now, a more accurate name could help development in the future if we look to use it in other places.

deadlycoconuts · 2022-11-03T05:59:29Z

Thanks a lot for the comments @krithika369, I've updated the PR to address them and I'll be merging this right now!

deadlycoconuts self-assigned this Oct 17, 2022

deadlycoconuts force-pushed the allow_changing_row_num_when_listing_exp branch 2 times, most recently from 8677027 to e8f635c Compare October 18, 2022 03:40

deadlycoconuts marked this pull request as ready for review October 19, 2022 02:09

deadlycoconuts commented Oct 19, 2022

View reviewed changes

deadlycoconuts requested a review from a team October 19, 2022 02:50

deadlycoconuts force-pushed the allow_changing_row_num_when_listing_exp branch from 3ab3658 to 200917a Compare October 19, 2022 03:14

krithika369 reviewed Oct 25, 2022

View reviewed changes

deadlycoconuts force-pushed the allow_changing_row_num_when_listing_exp branch 7 times, most recently from 6c50a04 to 4f5f1e0 Compare October 25, 2022 09:23

deadlycoconuts commented Oct 25, 2022

View reviewed changes

deadlycoconuts force-pushed the allow_changing_row_num_when_listing_exp branch from 4c3845e to 4a4fea0 Compare October 25, 2022 17:32

deadlycoconuts added 12 commits October 26, 2022 11:36

Add field selector to list experiments API

02a7571

Refactor experiment service by creating more helper functions

445de2f

Specify table fields when UI queries API for experiments list

1eeb15a

Fix refactoring changes

4df309d

Update experiment context to query more experiments at once

de9cf07

Remove all experiment fields from required field

5d81cce

Make list experiments return paginated responses when page values are…

e82df84

… not specified

Simplify api call to list experiments to only be performed once

5bccf98

Fix changes to experiment models struct

5fa5bd1

Update treatment service with changes to experiment model struct

7a6872b

Refactor pagination to only take place when fields are not specified

30302ec

Fix bug whereby query fields were not passed to experiment service

cec0c97

deadlycoconuts added 2 commits October 26, 2022 11:36

Make api call to display number of experiments retrieve only 1 field

3822e92

Simplify API calls to list experiments endpoint

b7bccab

deadlycoconuts force-pushed the allow_changing_row_num_when_listing_exp branch 2 times, most recently from c8158d6 to b7bccab Compare October 26, 2022 06:24

Fix bug whereby query fields were not passed to experiment service

8c701c0

deadlycoconuts commented Oct 26, 2022

View reviewed changes

Revert changes to experiments config group by modifying experiments c…

f22bddc

…ontroller behaviour

krithika369 approved these changes Oct 31, 2022

View reviewed changes

deadlycoconuts added 2 commits November 3, 2022 10:52

Refactor configs and contexts

09739a4

Refactor fieldNamesSet by using imported set

c990155

deadlycoconuts merged commit a2cf2c6 into caraml-dev:main Nov 3, 2022

deadlycoconuts deleted the allow_changing_row_num_when_listing_exp branch November 3, 2022 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add field selector to list experiments API #44

Add field selector to list experiments API #44

deadlycoconuts commented Oct 17, 2022 •

edited

Loading

deadlycoconuts Oct 19, 2022 •

edited

Loading

krithika369 Oct 19, 2022

krithika369 Oct 19, 2022

krithika369 Oct 19, 2022

deadlycoconuts Oct 25, 2022

krithika369 Oct 19, 2022

deadlycoconuts Oct 25, 2022

deadlycoconuts Oct 25, 2022

deadlycoconuts Oct 26, 2022

deadlycoconuts Oct 26, 2022 •

edited

Loading

deadlycoconuts Oct 26, 2022 •

edited

Loading

krithika369 left a comment •

edited

Loading

krithika369 Oct 31, 2022

deadlycoconuts Nov 3, 2022

krithika369 Nov 3, 2022

krithika369 Nov 3, 2022

deadlycoconuts Nov 3, 2022 •

edited

Loading

deadlycoconuts Nov 3, 2022 •

edited

Loading

krithika369 Oct 31, 2022

deadlycoconuts Nov 3, 2022

krithika369 Oct 31, 2022

deadlycoconuts commented Nov 3, 2022

Add field selector to list experiments API #44

Add field selector to list experiments API #44

Conversation

deadlycoconuts commented Oct 17, 2022 • edited Loading

Modifications

deadlycoconuts Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

deadlycoconuts Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

krithika369 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

deadlycoconuts Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts commented Nov 3, 2022

deadlycoconuts commented Oct 17, 2022 •

edited

Loading

deadlycoconuts Oct 19, 2022 •

edited

Loading

deadlycoconuts Oct 26, 2022 •

edited

Loading

deadlycoconuts Oct 26, 2022 •

edited

Loading

krithika369 left a comment •

edited

Loading

deadlycoconuts Nov 3, 2022 •

edited

Loading

deadlycoconuts Nov 3, 2022 •

edited

Loading